Fix magmap rank bug, magbin typo, plot.magbin speed; add tests and vignette sections#17
Merged
Merged
Conversation
…dead code, add tests and vignette sections Agent-Logs-Url: https://github.com/asgr/magicaxis/sessions/74e9c01b-f592-4d54-8e81-353d57d8b91c Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>
Agent-Logs-Url: https://github.com/asgr/magicaxis/sessions/74e9c01b-f592-4d54-8e81-353d57d8b91c Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a maintenance audit of the magicaxis package that fixes two silent bugs, removes dead code, and speeds up the inner drawing loop of plot.magbin. It also corrects several documentation typos and adds a substantial new test file plus vignette sections covering previously undocumented functions.
Changes:
- Bug fixes:
magmap(type='rank')now actually ranks values viarank(..., ties.method='average');magbinnow correctly assigns tooutput$nn.dists(the previousoutput$output$nn.distswas a silent no-op onNULL). - Performance/cleanup:
plot.magbinhoists the shape branch out of the per-bin loop and iterates only over non-NAcolmap$mapentries; dead.Last.magrojblock removed frommagband.R. - Docs & tests: typo fixes in
magclip.Rd,magmap.Rd,magrun.Rd(incl.bincen/binlim→bincens/binlims); newtests/testthat/test-functions.R(~50 tests) and new vignette sections formagclip,magrun,magerr,magcurve,magbar.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| R/magmap.R | Replace ineffective data[good][order(...)] = locut:hicut with rank(). |
| R/magbin.R | Fix output$output$nn.dists no-op; refactor plot.magbin draw loop for clarity/speed. |
| R/magband.R | Remove unused .Last.magroj local object. |
| man/magclip.Rd | Spelling and grammar corrections. |
| man/magmap.Rd | Typo: "sue" → "use". |
| man/magrun.Rd | Rename return items bincen/binlim to match code (bincens/binlims). |
| tests/testthat/test-functions.R | New unit tests for several previously untested functions and the rank bug regression. |
| vignettes/magicaxis.Rmd | New worked-example sections for magclip, magrun, magerr, magcurve, magbar. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ner alias for magtri too (since lots of people call them corner plots).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thorough audit of the package: two silent bugs, several doc/typo issues, a dead-code remnant, a speed improvement in the hot drawing loop, and materially expanded test and vignette coverage.
Bugs fixed
magmaptype='rank'produced no ranking (R/magmap.R)data[good][order(data[good])] = locut:hicutcreates an R temporary copy on the LHS —datawas never modified, so every point mapped identically. Replaced with vectorisedrank(..., ties.method='average').magbindistance deduplication was a no-op (R/magbin.Rline 226)output$output$nn.dists—output$outputisNULL, so the assignment silently did nothing. Fixed tooutput$nn.dists.Speed:
plot.magbinThe per-bin loop checked
x$shapeon every iteration (constant across all bins) and tested!is.na(colmap$map[i])in a conditional rather than pre-filtering. Refactored to select the drawing function once outside the loop and iterate only overwhich(!is.na(colmap$map)). Removed the now-redundant innercount > dustlimguard (bins are already filtered before the loop).Dead code removed
magband.R: unused.Last.magrojlocal object (a stale copy of.Last.magproj) removed.Documentation fixes
man/magclip.Rdman/magrun.Rdbincen/binlim→bincens/binlims(match code)man/magmap.RdTests added (
tests/testthat/test-functions.R)~50 new tests covering previously untested functions:
magclip,maghist,magrun(including the corrected return names),magerr,magcurve,magcon,magbar,magcutout, and a regression test for themagmaprank bug.Vignette additions
New worked-example sections in
vignettes/magicaxis.Rmd: magclip, magrun, magerr, magcurve, magbar.